Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add file with implicit call to blocking method #1941

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Add file with implicit call to blocking method #1941

merged 1 commit into from
Aug 27, 2024

Conversation

fedinskiy
Copy link
Contributor

@fedinskiy fedinskiy commented Aug 22, 2024 •
edited
Loading

Summary

Reproducer for this fix: quarkusio/quarkus#40721

How to reproduce:

cd http/rest-client-reactive
mvn clean compile quarkus:build -Dquarkus.platform.version=3.10.0 # this fails
mvn clean compile quarkus:build -Dquarkus.platform.version=3.11.0 # this works

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@fedinskiy fedinskiy requested a review from jcarranzan August 22, 2024 16:33
jcarranzan
jcarranzan reviewed Aug 26, 2024
View reviewed changes
http/rest-client-reactive/src/main/java/com/redhat/BrokenBlockingApi.java Outdated
@@ -0,0 +1,26 @@
package com.redhat; // this bug can not be reproduced in io.io.quarkus package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can it not be reproduced in our packages?

Copy link
Member

@michalvavrik michalvavrik Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read upstream review, fix PR and also don't get it. The comment needs to be improved to explain circumstances. I suppose all clients in io.quarkus are treated as builtin?

jcarranzan
jcarranzan reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@jcarranzan jcarranzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some questions.
Is it finished or is it a draft?
I tried to reproduce the gradle issue opened here with mvn but I didn't get it:
quarkusio/quarkus#38275 (comment)

michalvavrik
michalvavrik requested changes Aug 26, 2024
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please:

  • rebase on current main to fix CI
  • adding client without any test that calls it and without any link to the upstream issue won't do. I presume you expect build to fail when the client is present and so is bug, but:
    • you need to add comment so that we do understand what is tested
    • you need to add comment so that we do understand why is it not called
    • you need to link upstream issue

Preferably, but optionally, you add call with the newly added client as you never know when it might be useful (detecting bugs).

http/rest-client-reactive/src/main/java/com/redhat/BrokenBlockingApi.java Outdated
@@ -0,0 +1,26 @@
package com.redhat; // this bug can not be reproduced in io.io.quarkus package
Copy link
Member

@michalvavrik michalvavrik Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read upstream review, fix PR and also don't get it. The comment needs to be improved to explain circumstances. I suppose all clients in io.quarkus are treated as builtin?

@fedinskiy
Copy link
Contributor Author

@jcarranzan it is finished. The bug can be reproduced with maven, but only for quarkus 3.10 or older.
@michalvavrik added the required comments and rebased the PR. I did not add any test calls due to YAGNI principle, but I can add them, if you insist.

@michalvavrik
Copy link
Member

I did not add any test calls due to YAGNI principle, but I can add them, if you insist.

I don't insist, thanks for the changes. I don't quite agree with YAGNI in here but I have already explained my motivation for using the client, so let's keep your code as is.

@jcarranzan
Copy link
Contributor

jcarranzan commented Aug 27, 2024 •
edited
Loading

Ok @fedinskiy, could you put the maven command to know how to reproduce the bug? It would be useful to me, and then let's merge it, thank you.

@fedinskiy
Copy link
Contributor Author

@jcarranzan added the command(s) to the description

@jcarranzan jcarranzan self-requested a review August 27, 2024 08:47
@jcarranzan jcarranzan merged commit a2f4bc1 into quarkus-qe:main Aug 27, 2024
8 checks passed
jedla97 pushed a commit to jedla97/quarkus-test-suite that referenced this pull request Sep 3, 2024
@fedinskiy @jedla97
Add file with implicit call to blocking method (quarkus-qe#1941)
7d0b4dd 
Reproducer for this fix: quarkusio/quarkus#40721

(cherry picked from commit a2f4bc1)
@jedla97 jedla97 added this to the 3.8 milestone Sep 3, 2024
@jedla97 jedla97 mentioned this pull request Sep 3, 2024
Merged
9 tasks
rsvoboda pushed a commit that referenced this pull request Sep 6, 2024
@fedinskiy @rsvoboda
Add file with implicit call to blocking method (#1941)
d8f21cc 
Reproducer for this fix: quarkusio/quarkus#40721

(cherry picked from commit a2f4bc1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcarranzan jcarranzan jcarranzan approved these changes

@michalvavrik michalvavrik michalvavrik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8
Development

Successfully merging this pull request may close these issues.
4 participants
@fedinskiy @michalvavrik @jcarranzan @jedla97